7311: add GetReceiptsFromPeerTask #7638
Conversation
Signed-off-by: Matilda Clerke <matilda.clerke@consensys.net>
Signed-off-by: Matilda Clerke <matilda.clerke@consensys.net>
…e available Signed-off-by: Matilda Clerke <matilda.clerke@consensys.net>
Signed-off-by: Matilda Clerke <matilda.clerke@consensys.net>
…tantiation Signed-off-by: Matilda Clerke <matilda.clerke@consensys.net>
…en initializing PeerTaskFeatureToggle multiple times Signed-off-by: Matilda Clerke <matilda.clerke@consensys.net>
Signed-off-by: Matilda Clerke <matilda.clerke@consensys.net>
Signed-off-by: Matilda Clerke <matilda.clerke@consensys.net>
Signed-off-by: Matilda Clerke <matilda.clerke@consensys.net>
…tem' into 7311-add-GetReceiptsFromPeerTask
Signed-off-by: Matilda Clerke <matilda.clerke@consensys.net>
Signed-off-by: Matilda Clerke <matilda.clerke@consensys.net>
…tem' into 7311-add-GetReceiptsFromPeerTask
Signed-off-by: Matilda Clerke <matilda.clerke@consensys.net>
…tem' into 7311-add-GetReceiptsFromPeerTask
Signed-off-by: Matilda Clerke <matilda.clerke@consensys.net>
…tem' into 7311-add-GetReceiptsFromPeerTask
Signed-off-by: Matilda Clerke <matilda.clerke@consensys.net>
…tem' into 7311-add-GetReceiptsFromPeerTask
Signed-off-by: Matilda Clerke <matilda.clerke@consensys.net>
Signed-off-by: Matilda Clerke <matilda.clerke@consensys.net>
…peertask-system' into 7311-add-cli-feature-toggle-for-peertask-system
…tem' into 7311-add-GetReceiptsFromPeerTask
…' into 7311-add-GetReceiptsFromPeerTask
Signed-off-by: Matilda Clerke <matilda.clerke@consensys.net>
…tem' into 7311-add-GetReceiptsFromPeerTask
Signed-off-by: Matilda Clerke <matilda.clerke@consensys.net>
Signed-off-by: Matilda Clerke <matilda.clerke@consensys.net>
…-add-GetReceiptsFromPeerTask
Signed-off-by: Matilda Clerke <matilda.clerke@consensys.net>
…-add-GetReceiptsFromPeerTask
ethereum/core/src/main/java/org/hyperledger/besu/ethereum/mainnet/BodyValidator.java
Outdated
Show resolved
Hide resolved
| * @param syncState the sync state | ||
| * @param ethProtocolManager the eth protocol manager | ||
| * @param pivotBlockSelector the pivot block selector | ||
| * @return the synchronizer | ||
| */ | ||
| protected DefaultSynchronizer createSynchronizer( | ||
| final ProtocolSchedule protocolSchedule, | ||
| final Supplier<ProtocolSpec> currentProtocolSpecSupplier, |
There was a problem hiding this comment.
This shouldn't be needed we can just use the protocol schedule to get a ProtocolSpec
There was a problem hiding this comment.
I thought so too, but in order to get the ProtocolSpec from a ProtocolSchedule, you need a BlockHeader. The currentProtocolSpecSupplier always grabs the ProtocolSpec for the highest block we have.
Maybe it would make sense for this to be encapsulated in ProtocolSchedule with a getCurrentProtocolSpec method?
There was a problem hiding this comment.
As it turns out, trying to include this in the ProtocolSchedule ends up with even more code touched, and cyclic dependencies that make it very difficult to achieve
There was a problem hiding this comment.
Another way would be just to check that any forks are using POS in the protocol spec, no need to check every block header could do something like what I did the block import to check whether we are on a post merge chain https://github.com/hyperledger/besu/blob/main/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/sync/fastsync/FastSyncDownloadPipelineFactory.java#L122.
...in/java/org/hyperledger/besu/ethereum/eth/manager/peertask/task/GetReceiptsFromPeerTask.java
Outdated
Show resolved
Hide resolved
.../java/org/hyperledger/besu/ethereum/eth/sync/checkpointsync/CheckpointDownloadBlockStep.java
Outdated
Show resolved
Hide resolved
.../java/org/hyperledger/besu/ethereum/eth/sync/checkpointsync/CheckpointDownloadBlockStep.java
Outdated
Show resolved
Hide resolved
.../eth/src/main/java/org/hyperledger/besu/ethereum/eth/sync/fastsync/DownloadReceiptsStep.java
Show resolved
Hide resolved
| "Unexpectedly got receipts for block header already populated!"); | ||
| })); | ||
| } | ||
| // remove all the headers we found receipts for |
There was a problem hiding this comment.
The completion logic would be better in GetReceiptsFromPeerTask
There was a problem hiding this comment.
This code can only exist in the calling code as it's used to determine if we need to do another GetReceiptsFromPeerTask. When we discussed moving this logic into the GetReceiptsFromPeerTask, we realised that it would result in more confusing, recursive calls to the PeerTaskExecutor.
There was a problem hiding this comment.
I can see how that would make it more complex. This might be the best place to repeat the task for now.
Signed-off-by: Matilda Clerke <matilda.clerke@consensys.net>
… usages Signed-off-by: Matilda Clerke <matilda.clerke@consensys.net>
Signed-off-by: Matilda Clerke <matilda.clerke@consensys.net>
… update unit test to test for this functionality Signed-off-by: Matilda Clerke <matilda.clerke@consensys.net>
Signed-off-by: Matilda Clerke <matilda.clerke@consensys.net>
Signed-off-by: Matilda Clerke <matilda.clerke@consensys.net>
Signed-off-by: Matilda Clerke <matilda.clerke@consensys.net>
| * @param syncState the sync state | ||
| * @param ethProtocolManager the eth protocol manager | ||
| * @param pivotBlockSelector the pivot block selector | ||
| * @return the synchronizer | ||
| */ | ||
| protected DefaultSynchronizer createSynchronizer( | ||
| final ProtocolSchedule protocolSchedule, | ||
| final Supplier<ProtocolSpec> currentProtocolSpecSupplier, |
There was a problem hiding this comment.
Another way would be just to check that any forks are using POS in the protocol spec, no need to check every block header could do something like what I did the block import to check whether we are on a post merge chain https://github.com/hyperledger/besu/blob/main/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/sync/fastsync/FastSyncDownloadPipelineFactory.java#L122.
.../eth/src/main/java/org/hyperledger/besu/ethereum/eth/sync/fastsync/DownloadReceiptsStep.java
Outdated
Show resolved
Hide resolved
| "Unexpectedly got receipts for block header already populated!"); | ||
| })); | ||
| } | ||
| // remove all the headers we found receipts for |
There was a problem hiding this comment.
I can see how that would make it more complex. This might be the best place to repeat the task for now.
Signed-off-by: Matilda Clerke <matilda.clerke@consensys.net>
Signed-off-by: Matilda Clerke <matilda.clerke@consensys.net>
…PoS, remove new usages of currentProtocolSpecSupplier Signed-off-by: Matilda Clerke <matilda.clerke@consensys.net>
Signed-off-by: Matilda Clerke <matilda.clerke@consensys.net>
Signed-off-by: Matilda Clerke <matilda.clerke@consensys.net> Signed-off-by: Marlene Marz <m.marz@kabelmail.de>
PR description
This PR adds the GetReceiptsFromPeerTask PeerTask for all places the existing related EthTask is used and sets up addition and removal of peers for PeerManager